I ran make storeabi after building these bits, and expected the visibility of functions given the hidden visibility attributes to say as much, but they still show default visibility. Is that expected?

Hmm? They aren't exported anymore (as seen in <elf-function-symbols> and nm -D) – that's the intended result.

The <function-decl name='zprop_expand_list' filepath='../../include/libzfs_impl.h' line='159' column='1' visibility='default' binding='global' size-in-bits='64'> sexion probably attempts to mean something, but who knows what (the naming implies the visibility of the declaration, but that doesn't mean anything, especially in C), I've given up on trying to make sense of abigail output.

In talking with @ahrens about these changes, he had several questions about the use of attributes here in terms of maintainability and possible existing consumers, so I've asked him to elaborate on those.

That said, it looks like patches 2, 3 and 4 of the series in this PR could move ahead, if you'd like to break those out into another PR.

In terms of maintainability "hiding internal APIs" is generally preferred, I'd think, and this is one of a few breaking(ish) changes I've made to the libzfs ABIs for 2.1 (I think Rich had/has at least one too) – the sover needs a bump anyway, but I hope to, by doing a pass like this over libzfs (and others, time permitting), (a) sanitise the global symbol namespace a bit and (b) enforce the privacy boundary to (c) make further changes less likely to break users (but if they were using an internal API they were kinda asking for it). This will, for example, enable us to modify the handle at will, because there will be exactly 0 users that could possibly see it.

Moved middle three to #12116, will rebase this on that when it finishes building

libzfs is kind of a free-for-all, in that we don't commit to any of its interfaces, and the interface is not especially well designed (for example, the value of ZPOOL_STATUS_OK changes whenever a new status value is added). That said, I agree that it's valuable to make it harder to misuse, by not exporting symbols that should not be used by consumers.

I think it would be more maintainable to do that by defaulting to NOT export any symbols, and then explicitly annotating the symbols that we do want to make available. That way, when adding a new symbol we can't forget to add the "don't export" annotation and then have to remove the export in a later release. We can't forget to add the "yes export" annotation if it's used in our own libzfs consumers (/sbin/zfs, etc). If there's a symbol that we don't consume outside the library but it's useful to others, we can always add the "yes export" annotation later.

From reading the gcc documentation, it looks like this is done by compiling with -fvisibility=hidden, and then annotating symbols that we want to export with __attribute__ ((visibility ("default"))) (preferably via a macro). Of course we would want to find a way to do this on all supported build targets, including llvm (which is used on FreeBSD).

You raise some good points, and that's definitely a better approach than what I wrote originally – see new HEAD (it's on top of #12116 by necessity).

The symbol table changed as such:

diff --git a/pre b/post
index be55f21..dc2e21f 100755
--- a/pre
+++ b/post
@@ -205,7 +205,6 @@ U mmap64
 U mount
 T mountpoint_namecheck
 U munmap
-D nfs_only
 U nvlist_add_boolean
 U nvlist_add_nvlist
 U nvlist_add_nvlist_array
@@ -280,17 +279,9 @@ T sa_errorstr
 T sa_is_shared
 T sa_validate_shareopts
 U sched_yield
-T SHA256Init
-T SHA2Final
-T SHA2Init
-T SHA2Update
-T SHA384Init
-T SHA512Init
-D share_all_proto
 U sigaction
 U sigemptyset
 U sleep
-D smb_only
 B smb_shares
 T snapshot_namecheck
 U snprintf
@@ -418,7 +409,6 @@ T zfs_ioctl
 T zfs_is_mounted
 T zfs_is_shared
 T zfs_is_shared_nfs
-T zfs_is_shared_proto
 T zfs_is_shared_smb
 T zfs_iter_bookmarks
 T zfs_iter_children
@@ -578,7 +568,6 @@ T zpool_label_disk
 U zpool_label_disk_wait
 T zpool_load_compat
 T zpool_log_history
-W zpool_mount_datasets
 T zpool_name_to_prop
 T zpool_obj_to_path
 T zpool_obj_to_path_ds
@@ -615,7 +604,6 @@ T zpool_skip_pool
 T zpool_state_to_name
 T zpool_sync_one
 T zpool_trim
-W zpool_unmount_datasets
 T zpool_upgrade
 T zpool_vdev_attach
 T zpool_vdev_clear

This is much cleaner implementation-wise, too!

There's some symbol leakage from libshare et al, but that could probably go in a PR after this one and #12050.

I was very conservative when it came to trimming out symbols – the crypto code (and headers that export one symbol) isn't necessarily consistent internally, as much as it is consistent with What Happened To Be Exported Before.